Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert softmax from CTest to GTest #3479

Merged
merged 12 commits into from
Feb 10, 2025

Conversation

Vsevolod1983
Copy link
Contributor

@Vsevolod1983 Vsevolod1983 commented Jan 22, 2025

CTest to GTest conversion for Softmax
soft_max.cpp file is detected as renamed (moved), so probably will not be easy to review.

@Vsevolod1983 Vsevolod1983 marked this pull request as draft January 23, 2025 16:03
@Vsevolod1983 Vsevolod1983 marked this pull request as ready for review January 23, 2025 17:51
CompareResults(tensorGpuDataBackward, tensorCpuDataBackward);
}

std::vector<T> GetForwardCpu() const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@bghimireamd bghimireamd Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this one is parallel may be we can move this mloSoftmaxForwardRunHost and mloSoftmaxBackwardRunHost to avoid duplications.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, @bghimireamd
In C-test I did not see calls of these functions. Scope of my refactoring was to replace c-test with g-test.
If I misunderstand something, lets discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you mean that we need to enhance test coverage for softmax? If yes I would prefer to do it in separate PR/ticket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah separate PR/ticket make more sense here :)

Copy link
Contributor

@bghimireamd bghimireamd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Vsevolod1983 Vsevolod1983 merged commit 39b5fc5 into develop Feb 10, 2025
29 of 145 checks passed
@Vsevolod1983 Vsevolod1983 deleted the vgolovko/ConvertSoftmaxToGTest branch February 10, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants